-
Notifications
You must be signed in to change notification settings - Fork 1
set seconds to zero on create time-entry #399 #416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0b7c0cf to
7849d48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @Angeluz-07 . 💯
It would be better if you add some unit tests around these new feature
| const endDateChanged = this.entry.end_date !== event.entry.end_date; | ||
|
|
||
| if (startDateChanged) { | ||
| const startDate = new Date(event.entry.start_date); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 71, 72, 73 are the same than 89,90,91. Do not you think it would be great if we extract them to a method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your're right. I thought the same. I just didn't know the code style to include functions. I will ask for help to Rene for this and also to fix the tests. Thanks for pointing it out.
0df5437 to
82a4ff9
Compare
Codecov Report
@@ Coverage Diff @@
## master #416 +/- ##
==========================================
- Coverage 90.66% 90.36% -0.31%
==========================================
Files 72 72
Lines 1286 1297 +11
Branches 83 87 +4
==========================================
+ Hits 1166 1172 +6
- Misses 93 96 +3
- Partials 27 29 +2
Continue to review full report at Codecov.
|
fixes #399